- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 8.6k
[grid] Improve SlotMatcher and SlotSelector on request browserVersion #14914
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| PR Reviewer Guide 🔍Here are some key observations to aid the review process: 
 | 
| PR Code Suggestions ✨Explore these optional code suggestions: 
 | 
| CI Failure Feedback 🧐(Checks updated until commit ab99d84)
 | 
        
          
                java/src/org/openqa/selenium/grid/distributor/selector/DefaultSlotSelector.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      00449d7    to
    7369c6a      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have left some ideas for improvments, i hope this helps.
        
          
                java/src/org/openqa/selenium/grid/data/CapabilitiesComparator.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                java/src/org/openqa/selenium/grid/data/CapabilitiesComparator.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      246d1d1    to
    b2d2683      
    Compare
  
    b2d2683    to
    ccc62a9      
    Compare
  
    | Thanks @joerg1985, can you check is it better now? | 
| @VietND96 i think this comparator is not suitable for sorting the node slots. e.g.: Update: with valid input this returns good results, sorry for the confusion. | 
        
          
                java/src/org/openqa/selenium/grid/data/SemanticVersionComparator.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      Signed-off-by: Viet Nguyen Duc <[email protected]>
ccc62a9    to
    d5a0fc6      
    Compare
  
    | Codecov ReportAll modified and coverable lines are covered by tests ✅ 
 Additional details and impacted files@@            Coverage Diff             @@
##            trunk   #14914      +/-   ##
==========================================
+ Coverage   58.48%   59.30%   +0.82%     
==========================================
  Files          86       94       +8     
  Lines        5270     6040     +770     
  Branches      220      268      +48     
==========================================
+ Hits         3082     3582     +500     
- Misses       1968     2190     +222     
- Partials      220      268      +48     ☔ View full report in Codecov by Sentry. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix https://github.com/SeleniumHQ/selenium/actions/runs/12504197675/job/34885613546?pr=14914#step:17:1469 and then we can merge.
Signed-off-by: Viet Nguyen Duc <[email protected]>
…SeleniumHQ#14914) * [grid] Improve SlotMatcher and SlotSelector on request browserVersion Signed-off-by: Viet Nguyen Duc <[email protected]> * Fix tests Signed-off-by: Viet Nguyen Duc <[email protected]> --------- Signed-off-by: Viet Nguyen Duc <[email protected]> Co-authored-by: Diego Molina <[email protected]>
User description
Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it
Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.
Description
This improvement helps autoscaling control strictly on a request without the capability to set
browserVersionwill be assigned to list Slots with top priority on stereotype withoutbrowserVersionor a latestbrowserVersion(based on SemVer comparison).Improve DefaultSlotMatcher, where the request with cap
browserVersion=130can match to slot with stereotypebrowserVersion=130.0(based on SemVer comparison return 0). Few corner case also covered e.gMotivation and Context
This pull request introduces several important changes to the Selenium Grid project, focusing on improving the way browser versions are compared and sorted. The changes include implementing a custom semantic version comparator, updating the slot selection logic, and adding relevant tests.
Improvements to browser version comparison:
java/src/org/openqa/selenium/grid/data/NodeStatus.java: Added agetBrowserVersionmethod and a customsemVerComparatormethod for comparing semantic versions with empty strings considered first.Updates to slot selection logic:
java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java: Modified thematchesmethod to use the newsemVerComparatorfor browser version comparison.java/src/org/openqa/selenium/grid/distributor/selector/DefaultSlotSelector.java: Updated the slot selection logic to sort nodes by browser version using the new comparator.Addition of tests:
java/test/org/openqa/selenium/grid/data/NodeStatusTest.java: Added tests for thesemVerComparatormethod to ensure correct browser version matching.java/test/org/openqa/selenium/grid/distributor/selector/DefaultSlotSelectorTest.java: Added tests to verify nodes are ordered correctly by browser version. [1] [2]Miscellaneous:
rust/src/lock.rs: Added license information to the file.Types of changes
Checklist
PR Type
Enhancement
Description
Enhanced browser version handling in Selenium Grid with the following improvements:
Implemented semantic version comparison for better browser version matching
Improved slot selection and matching logic:
Added comprehensive test coverage:
Code quality improvements:
Changes walkthrough 📝
DefaultSlotMatcher.java
Improved browser version matching logicjava/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java
semVerComparatorfor moreflexible matching
NodeStatus.java
Added semantic version comparison functionalityjava/src/org/openqa/selenium/grid/data/NodeStatus.java
getBrowserVersionmethod to retrieve minimum browser versionsemVerComparatorfor semantic version comparisonisNumberfor version parsingDefaultSlotSelector.java
Enhanced slot selection with version-based sortingjava/src/org/openqa/selenium/grid/distributor/selector/DefaultSlotSelector.java
NodeStatusTest.java
Added tests for semantic version comparisonjava/test/org/openqa/selenium/grid/data/NodeStatusTest.java
DefaultSlotSelectorTest.java
Added tests for version-based node orderingjava/test/org/openqa/selenium/grid/distributor/selector/DefaultSlotSelectorTest.java
lock.rs
Added missing license headerrust/src/lock.rs